❮

Improve description of XLOG_RUNNING_XACTS

  • Jump to comment-1
    sawada.mshk@gmail.com2022-07-21T01:13:25+00:00
    Hi, I realized that standby_desc_running_xacts() in standbydesc.c doesn't describe subtransaction XIDs. I've attached the patch to improve the description. Here is an example by pg_wlaldump: * HEAD rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 * w/ patch rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 subxacts: 1049 Please review it. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
    • Jump to comment-1
      ashutosh.bapat.oss@gmail.com2022-07-21T13:12:53+00:00
      Hi On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > describe subtransaction XIDs. I've attached the patch to improve the > description. Here is an example by pg_wlaldump: > > * HEAD > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 > > * w/ patch > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 > subxacts: 1049 > I think this is a good addition to debugging info. +1 If we are going to add 64 subxid numbers then it would help if we could be more verbose and print "subxid overflowed" instead of "subxid ovf". -- Best Wishes, Ashutosh Bapat
      • Jump to comment-1
        sawada.mshk@gmail.com2022-07-28T02:11:15+00:00
        On Thu, Jul 21, 2022 at 10:13 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote: > > Hi > > On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > Hi, > > > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > describe subtransaction XIDs. I've attached the patch to improve the > > description. Here is an example by pg_wlaldump: > > > > * HEAD > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 > > > > * w/ patch > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 > > subxacts: 1049 > > > > I think this is a good addition to debugging info. +1 > > If we are going to add 64 subxid numbers then it would help if we > could be more verbose and print "subxid overflowed" instead of "subxid > ovf". Yeah, it looks better so agreed. I've attached an updated patch. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
        • Jump to comment-1
          ashutosh.bapat@enterprisedb.com2022-07-28T04:26:33+00:00
          Thanks Masahiko for the updated patch. It looks good to me. I wonder whether the logic should be, similar to ProcArrayApplyRecoveryInfo() if (xlrec->subxid_overflow) ... else if (xlrec->subxcnt > 0) ... But you may ignore it. -- Best Wishes, Ashutosh On Thu, Jul 28, 2022 at 7:41 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Thu, Jul 21, 2022 at 10:13 PM Ashutosh Bapat > <ashutosh.bapat.oss@gmail.com> wrote: > > > > Hi > > > > On Thu, Jul 21, 2022 at 6:44 AM Masahiko Sawada <sawada.mshk@gmail.com> > wrote: > > > > > > Hi, > > > > > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > > describe subtransaction XIDs. I've attached the patch to improve the > > > description. Here is an example by pg_wlaldump: > > > > > > * HEAD > > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048 > > > > > > * w/ patch > > > rmgr: Standby len (rec/tot): 58/ 58, tx: 0, lsn: > > > 0/01D0C608, prev 0/01D0C5D8, desc: RUNNING_XACTS nextXid 1050 > > > latestCompletedXid 1047 oldestRunningXid 1048; 1 xacts: 1048; 1 > > > subxacts: 1049 > > > > > > > I think this is a good addition to debugging info. +1 > > > > If we are going to add 64 subxid numbers then it would help if we > > could be more verbose and print "subxid overflowed" instead of "subxid > > ovf". > > Yeah, it looks better so agreed. I've attached an updated patch. > > Regards, > > -- > Masahiko Sawada > EDB: https://www.enterprisedb.com/ >
          • Jump to comment-1
            horikyota.ntt@gmail.com2022-07-28T06:24:54+00:00
            At Thu, 28 Jul 2022 09:56:33 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in > Thanks Masahiko for the updated patch. It looks good to me. > > I wonder whether the logic should be, similar > to ProcArrayApplyRecoveryInfo() > if (xlrec->subxid_overflow) > ... > else if (xlrec->subxcnt > 0) > ... > > But you may ignore it. Either is fine if we asuume the record is sound, but since it is debugging output, I think we should always output the information *for both* . The following change doesn't change the output for a sound record. ==== if (xlrec->subxcnt > 0) { appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt); for (i = 0; i < xlrec->subxcnt; i++) appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]); } - else if (xlrec->subxid_overflow) + if (xlrec->subxid_overflow) appendStringInfoString(buf, "; subxid overflowed"); ==== Another point is if the xid/subxid lists get long, I see it annoying that the "overflowed" messages goes far away to the end of the long line. Couldn't we rearrange the item order of the line as the follows? nextXid %u latestCompletedXid %u oldestRunningXid %u;[ subxid overflowed;][ %d xacts: %u %u ...;][ subxacts: %u %u ..] regards. -- Kyotaro Horiguchi NTT Open Source Software Center
            • Jump to comment-1
              sawada.mshk@gmail.com2022-07-28T06:53:33+00:00
              On Thu, Jul 28, 2022 at 3:24 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 28 Jul 2022 09:56:33 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in > > Thanks Masahiko for the updated patch. It looks good to me. > > > > I wonder whether the logic should be, similar > > to ProcArrayApplyRecoveryInfo() > > if (xlrec->subxid_overflow) > > ... > > else if (xlrec->subxcnt > 0) > > ... > > > > But you may ignore it. > > Either is fine if we asuume the record is sound, but since it is > debugging output, I think we should always output the information *for > both* . The following change doesn't change the output for a sound > record. > > ==== > if (xlrec->subxcnt > 0) > { > appendStringInfo(buf, "; %d subxacts:", xlrec->subxcnt); > for (i = 0; i < xlrec->subxcnt; i++) > appendStringInfo(buf, " %u", xlrec->xids[xlrec->xcnt + i]); > } > - else if (xlrec->subxid_overflow) > + if (xlrec->subxid_overflow) > appendStringInfoString(buf, "; subxid overflowed"); > ==== Do you mean that both could be true at the same time? If I read GetRunningTransactionData() correctly, that doesn't happen. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
              • Jump to comment-1
                horikyota.ntt@gmail.com2022-07-28T07:29:32+00:00
                At Thu, 28 Jul 2022 15:53:33 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > > Do you mean that both could be true at the same time? If I read > GetRunningTransactionData() correctly, that doesn't happen. So, I wrote "since it is debugging output", and "fine if we asuume the record is sound". Is it any trouble with assuming the both *can* happen at once? If something's broken, it will be reflected in the output. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
    • Jump to comment-1
      masao.fujii@oss.nttdata.com2022-07-21T02:21:09+00:00
      On 2022/07/21 10:13, Masahiko Sawada wrote: > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > describe subtransaction XIDs. I've attached the patch to improve the > description. +1 The patch looks good to me. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
      • Jump to comment-1
        horikyota.ntt@gmail.com2022-07-21T07:29:44+00:00
        At Thu, 21 Jul 2022 11:21:09 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > On 2022/07/21 10:13, Masahiko Sawada wrote: > > Hi, > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > describe subtransaction XIDs. I've attached the patch to improve the > > description. > > +1 > > The patch looks good to me. The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit also shows subtransactions but they are at maximum 64. I feel like -0.3 if there's no obvious advantage showing them. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
        • Jump to comment-1
          sawada.mshk@gmail.com2022-07-21T07:58:45+00:00
          On Thu, Jul 21, 2022 at 4:29 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > > At Thu, 21 Jul 2022 11:21:09 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in > > > > > > On 2022/07/21 10:13, Masahiko Sawada wrote: > > > Hi, > > > I realized that standby_desc_running_xacts() in standbydesc.c doesn't > > > describe subtransaction XIDs. I've attached the patch to improve the > > > description. > > > > +1 > > > > The patch looks good to me. > > The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = > PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit > also shows subtransactions but they are at maximum 64. I feel like > -0.3 if there's no obvious advantage showing them. xxx_desc() functions are debugging purpose functions as they are used by pg_waldump and pg_walinspect etc. I think such functions should show all contents unless there is reason to hide them. Particularly, standby_desc_running_xacts() currently shows subtransaction information only when subtransactions are overflowed, which got me confused when inspecting WAL records. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
          • Jump to comment-1
            horikyota.ntt@gmail.com2022-07-21T08:28:40+00:00
            At Thu, 21 Jul 2022 16:58:45 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in > > > The patch looks good to me. By the way +1 to this. > > The subxids can reach TOTAL_MAX_CACHED_SUBXIDS = > > PGPROC_MAX_CACHED_SUBXIDS(=64) * PROCARRAY_MAXPROCS. xact_desc_commit > > also shows subtransactions but they are at maximum 64. I feel like > > -0.3 if there's no obvious advantage showing them. > > xxx_desc() functions are debugging purpose functions as they are used > by pg_waldump and pg_walinspect etc. I think such functions should > show all contents unless there is reason to hide them. Particularly, > standby_desc_running_xacts() currently shows subtransaction > information only when subtransactions are overflowed, which got me > confused when inspecting WAL records. I'm not sure just confusing can justify that but after finding logicalmsg_desc dumps the whole content, I no longer feel against to show subxacts. Just for information, but as far as I saw, relmap_desc shows only the length of "data" but doesn't dump all of it. generic_desc behaves the same way. Thus we could just show "%d subxacts" instead of dumping out the all subxact ids just to avoid that confusion. However, again, I no longer object to show all subxids. regards. -- Kyotaro Horiguchi NTT Open Source Software Center